Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Accept tar and zip headers in contrib.utils.download requests #1704

Merged
merged 2 commits into from
Nov 17, 2021

Conversation

matthewfeickert
Copy link
Member

Description

From #1697 (comment) @GraemeWatt notes:

The Accept headers will be ignored if archive_url is a HEPData URL (ending in ?view=true) that returns the content directly. The Accept headers will only become relevant when archive_url is a HEPData DOI for a resource file after we redirect to a landing page. We're still working on the relevant PR (HEPData/hepdata#412) after it expanded in scope. The default Accept header is */* for Python requests (and curl), different from the default Accept values of most web browsers where */* has a lower weighting (quality value) than text/html. With our current implementation, if */* has weight 1 in a request to the landing page, the content will be returned directly instead of the HTML landing page. This means you don't strictly need to specify the Accept header with Python requests. But I'd recommend you keep the Accept header to avoid accepting content types that your download code can't process. To accept both tar and zip files, you just need to use headers={"Accept": "application/x-tar, application/zip"}. We will return a 406 Not Acceptable status code if a request is made to the landing page where the Accept header does not match the media type of the content.

Checklist Before Requesting Reviewer

  • Tests are passing
  • "WIP" removed from the title of the pull request
  • Selected an Assignee for the PR to be responsible for the log summary

Before Merging

For the PR Assignees:

  • Summarize commit messages into a comprehensive review of the PR
* Add zip headers to the accepted content headers for contrib.utils.download
* Amends PR #1697

Co-authored-by: Graeme Watt <[email protected]>

@matthewfeickert matthewfeickert added fix A bug fix contrib Targeting pyhf.contrib and not the core of pyhf labels Nov 17, 2021
@matthewfeickert matthewfeickert self-assigned this Nov 17, 2021
@matthewfeickert matthewfeickert changed the title fix: Accept for tar and zip headers in contrib.utils.download requests fix: Accept tar and zip headers in contrib.utils.download requests Nov 17, 2021
@codecov
Copy link

codecov bot commented Nov 17, 2021

Codecov Report

Merging #1704 (c9cf7c0) into master (d4b2d9c) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1704   +/-   ##
=======================================
  Coverage   98.11%   98.11%           
=======================================
  Files          64       64           
  Lines        4246     4246           
  Branches      591      591           
=======================================
  Hits         4166     4166           
  Misses         46       46           
  Partials       34       34           
Flag Coverage Δ
contrib 26.28% <ø> (ø)
doctest 61.02% <ø> (ø)
unittests 96.16% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/pyhf/contrib/utils.py 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d4b2d9c...c9cf7c0. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contrib Targeting pyhf.contrib and not the core of pyhf fix A bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants